-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(voicea-highlight): added-highlights #3807
Conversation
@@ -183,6 +184,17 @@ export type CaptionData = { | |||
speaker: string; | |||
}; | |||
|
|||
export type Highlight = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need a new type for highlights object.
@@ -365,6 +365,7 @@ export const EVENT_TRIGGERS = { | |||
MEETING_STOPPED_RECEIVING_TRANSCRIPTION: 'meeting:receiveTranscription:stopped', | |||
MEETING_MANUAL_CAPTION_UPDATED: 'meeting:manualCaptionControl:updated', | |||
MEETING_CAPTION_RECEIVED: 'meeting:caption-received', | |||
MEETING_HIGHLIGHT_CREATED: 'meeting:highlight-created', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New event to be emitted from meeting object to the user.
* @public | ||
* @returns {Promise<void>} a promise to open the WebSocket connection | ||
*/ | ||
public async toggleHighlighting(activate: boolean, options?: {spokenLanguage?: string}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a separate stop
or start
method, we can use toggle
which enabled users to toggle the highlighting setting. This way we can also compact all the logic inside of this method.
}); | ||
} | ||
} else { | ||
throw new Error('This operation is not allowed in your org. Contact org administrator.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We throw this error if we find that transcriptions are not enabled. Usually happens if the old webex assistant is not enabled for the org.
@@ -118,3 +121,41 @@ export const processNewCaptions = ({ | |||
} | |||
transcriptData.interimCaptions[transcriptId] = interimTranscriptionIds; | |||
}; | |||
|
|||
export const processHighlightCreated = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We simply process the highlights voicea gives back and pack it into the meetings object as an array.
@@ -1188,6 +1188,49 @@ describe('plugin-meetings', () => { | |||
}); | |||
}); | |||
|
|||
describe('#toggleHighlighting', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests to check all the flows (toggling). Had to mock a lot of data.
|
||
describe('processHighlightCreated', () => { | ||
beforeEach(() => { | ||
fakeVoiceaPayload = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too - had to mock a lot of values in order to properly unit test the required method.
@@ -1104,6 +1106,44 @@ async function toggleTranscription(enable = false){ | |||
} | |||
} | |||
|
|||
async function toggleHighlights() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using toggle is an easier way for users to access highlighting.
Here, we check for the button data status and if it is set as 'true', it indicated that it was turned on and hence needs to be switched off - hence the first block is that flow while the second block is the flow for turning on the highlights.
@@ -1144,6 +1184,9 @@ function setTranscriptEvents() { | |||
meeting.on('meeting:receiveTranscription:stopped', () => { | |||
generalToggleTranscription.innerText = "Start Transcription"; | |||
generalTranscriptionContent.innerHTML = 'Transcription Content: Webex Assistant must be enabled, check the console!'; | |||
generalHighlightTranscription.setAttribute('data-enabled', "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMPORTANT - Here we need to disable the transcription button and replace the text as when transcription is stopped highlighting no longer sends events.
Also - this gets triggered when we close the meeting also.
I'm not a 100% sure about this method (open to any suggestions). However, since this works for the sample app - I feel it should work.
@@ -851,13 +851,17 @@ <h2 class="collapsible"> | |||
<div class="btn-group u-mb"> | |||
<button id="gc-toggle-transcription" type="button" | |||
onclick="toggleTranscription()" class="btn-code" data-enabled="false">Start Transcription</button> | |||
<button id="gc-toggle-highlights" type="button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating new UI for this change.
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #< SPARK-509136 >
This pull request addresses
by making the following changes
Screenshot of highlights object
Screenshot of Sample App
Change Type
The following scenarios where tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.